Skip to content

fix(editor): cascade clip modifier deletion - #242

Merged
EtienneLescot merged 2 commits into
getopenscreen:mainfrom
arhxam:codex/cascade-clip-modifier-deletion
Aug 4, 2026
Merged

fix(editor): cascade clip modifier deletion#242
EtienneLescot merged 2 commits into
getopenscreen:mainfrom
arhxam:codex/cascade-clip-modifier-deletion

Conversation

@arhxam

@arhxam arhxam commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • remove zoom, annotation, speed, and camera-fullscreen regions anchored to a deleted clip even when it is the final clip
  • route asset deletion through the shared clip-removal operation so surviving clips reflow and their derived modifier positions stay correct
  • preserve legacy unanchored regions while deleting only content tied to removed clips
  • add document and persisted-service regression coverage

Related issue

No linked issue; found while auditing destructive timeline operations for orphaned state.

Type of change

  • Bug fix
  • Feature
  • Enhancement
  • Documentation
  • Refactor / maintenance
  • Performance
  • Security

Release impact

  • Patch
  • Minor
  • Major / breaking change
  • No release note needed

Desktop impact

  • Windows
  • macOS
  • Linux
  • Installer / packaging
  • Not platform-specific

Screenshots / video

Not applicable; this fixes document cleanup and timeline math.

Testing

  • npm exec -- vitest run src/lib/ai-edition/document/timeline.test.ts electron/ai-edition/document-service.test.ts
  • npm exec -- biome check src/lib/ai-edition/document/timeline.ts src/lib/ai-edition/document/timeline.test.ts electron/ai-edition/document-service.ts electron/ai-edition/document-service.test.ts
  • npm run build-vite
  • npm run wb:typecheck

Summary by CodeRabbit

  • Bug Fixes
    • Fixed asset and clip removal so associated timeline modifiers—such as zooms, annotations, speed regions, camera regions, and trims—are cleaned up correctly.
    • Preserved legacy, unanchored timeline regions when their related clips are removed.
    • Improved behavior when deleting the final clip from a timeline.

@arhxam
arhxam requested a review from EtienneLescot as a code owner August 4, 2026 00:17
@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 860886c8-40a3-43cb-a61d-59dac04e00e3

📥 Commits

Reviewing files that changed from the base of the PR and between 545043d and 4aaf4d9.

📒 Files selected for processing (4)
  • electron/ai-edition/document-service.test.ts
  • electron/ai-edition/document-service.ts
  • src/lib/ai-edition/document/timeline.test.ts
  • src/lib/ai-edition/document/timeline.ts

📝 Walkthrough

Walkthrough

removeClip now removes clip-anchored modifiers, including when the deleted clip is the last clip. removeAsset uses this cleanup for associated clips. Tests cover zoom ranges, speed regions, annotations, camera-fullscreen regions, trims, and legacy unanchored regions.

Changes

Timeline cleanup

Layer / File(s) Summary
Clip removal cleanup
src/lib/ai-edition/document/timeline.ts, src/lib/ai-edition/document/timeline.test.ts
removeClip removes regions anchored to the deleted clip before re-deriving remaining regions. The regression test verifies cleanup of anchored modifiers and preservation of unanchored legacy regions.
Asset removal integration
electron/ai-edition/document-service.ts, electron/ai-edition/document-service.test.ts
removeAsset delegates clip deletion to removeClip before updating trim ranges. The test verifies cascading removal of clip modifiers.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Suggested reviewers: etiennelescot

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: cascading deletion of clip modifiers.
Description check ✅ Passed The description includes all template sections and provides clear change details, release impact, platform impact, and testing information.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@EtienneLescot EtienneLescot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the strongest PR in the batch, and the reason is a path neither the description nor CodeRabbit names.

The old guard was oldClips.length > 0 && newClips.length > 0. The first half was dead — arr.length !== oldClips.length already implies oldClips.length >= 1. The second half is the bug: delete the last clip and every anchored zoom, annotation, speed and camera region survives with clipId still pointing at the clip that just went away, and useTimeline.ts:977 writes that straight to disk.

It doesn't stay dangling. Reopen the project: Preview mounts the asset even with an empty timeline, handleLoadedMetadata sees clips.length === 0 and calls replaceTimeline to seed a full-duration clip (NewEditorShell.tsx:364-380). reconcileRegionsAfterReplace then finds those orphans anchored to a clip that isn't in surviving, falls through to anchorRegionsWithDerivedMs, and re-ventilates them from their stale ruler ms onto the new clip. placed comes back true, so they're kept. The zooms the user deleted reappear, at whatever source time their old ms happens to land on. Your change is what stops that.

I re-checked this against the region/pill work that landed since your merge base — no interaction. Clip deletion is only reachable from the per-clip trash button, which renders only while the clip is selected, and selectClip clears the pill selection first.

Merging. Two notes inline, neither blocking.

Comment thread src/lib/ai-edition/document/timeline.ts
Comment thread electron/ai-edition/document-service.ts

@EtienneLescot EtienneLescot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threads cleared — the isAnchored asymmetry is unreachable on current main and the multi-asset coverage gap is tracked in #249, neither worth holding this for.

Approving. This is the one I most want in the tree: deleting the last clip left every anchored zoom, annotation, speed and camera region pointing at a clip that no longer existed, and the next project open reseeded a clip and re-ventilated them onto new footage. Deleted pills coming back on top of different video is about as bad as an editor bug gets, and the two new tests fail on main and pass here.

@EtienneLescot
EtienneLescot merged commit c186236 into getopenscreen:main Aug 4, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants